Add SLH-DSA Support - #455

🟠 [High] wp_internal.c fails to compile when WP_HAVE_PKCS8_ENC is undefined: WP_CIPHER_NAMES_LEN is used outside the #ifdef that defines it
🚫 BLOCK bug

The PR wraps both wp_cipher_names[] (line 891) and the #define WP_CIPHER_NAMES_LEN (line 902) in #ifdef WP_HAVE_PKCS8_ENC ... #endif (closing line 904), but the two uses of WP_CIPHER_NAMES_LEN are NOT inside that guard: the initializer at line 943 (size_t i = WP_CIPHER_NAMES_LEN;) and the found/not-found test at line 961. Only the for-loop at 945-955 is guarded. When WP_HAVE_PKCS8_ENC is undefined the macro does not exist and the translation unit fails to compile with an undeclared-identifier error.

The configuration is reachable: settings.h defines WP_HAVE_PKCS8_ENC only for defined(HAVE_PKCS8) && !defined(NO_PWDBASED), so a wolfSSL built --disable-pwdbased or --disable-pkcs8 leaves it undefined. Nothing #errors on that combination and wp_internal.c is unconditionally in the build (src/include.am). The comment at 958-960 ("Unknown cipher, or a build that cannot encrypt keys at all") shows the case was meant to be supported — and this macro gates the entire cipher-on-PKI feature added across the kmgmt files.

The found/not-found logic itself is correct: the loop breaks with i < WP_CIPHER_NAMES_LEN, so a match at the last index is not misread. The defect is purely guard placement. Confirmed by three independent traces.

Suggestion:

Recommendation: Add an #else arm defining WP_CIPHER_NAMES_LEN as 0 so the sentinel comparison still works, or restructure with a local int found = 0;. Add a CI compile job with a --disable-pwdbased wolfSSL.

🟡 [Medium] pem2der accepts ENCRYPTED PRIVATE KEY PEM in non-WOLFSSL_ENCRYPTED_KEYS builds and mislabels it as PrivateKeyInfo
💡 SUGGEST bug

Moving #ifdef WOLFSSL_ENCRYPTED_KEYS from around the whole branch to just the info.passwd_cb assignments is required for compilation (EncryptedInfo only carries passwd_cb under that macro), but it changes runtime behavior, and the new comment is inaccurate.

In wolfSSL's PemToDer, the entire decrypt block for header == BEGIN_ENC_PRIV_KEY sits inside #ifdef WOLFSSL_ENCRYPTED_KEYS. Without that macro, wc_PemToDer base64-decodes the body and returns 0 with the EncryptedPrivateKeyInfo DER intact. wp_pem2der_decode_data then reports data-structure = "PrivateKeyInfo" (line 326), so OpenSSL routes it to a decoder that cannot parse an EncryptedPrivateKeyInfo. Meanwhile the wp_epki_to_pki decoder registered for structure EncryptedPrivateKeyInfo — verified at src/wp_wolfprov.c:1645, needing only HAVE_PKCS8 — is never reached.

Before the PR this input hit else { ok = 0; } and failed cleanly. The comment is wrong in both directions: with WOLFSSL_ENCRYPTED_KEYS, wc_PemToDer decrypts in place via passwd_cb; without it, nothing downstream sees an EPKI label. Net effect: the change does not enable encrypted-PEM loading on the FIPS-style builds it targets — it replaces a clean rejection with a confusing downstream parse failure.

Separately, the new #ifdef at line 332 sits at column 0 while sibling guards in the same function (275, 297) use the repo's #ifdef indentation.

Suggestion:

Recommendation: Report dataFormat = "EncryptedPrivateKeyInfo" when WOLFSSL_ENCRYPTED_KEYS is undefined, keep "PrivateKeyInfo" when defined, fix the comment, and indent the guard to match.

🟡 [Medium] Raw-message mode and the other signature params are now sticky across re-init, contradicting the ML-DSA sibling
💡 SUGGEST bug

wp_slhdsa_init resets only the message buffer and staged verify signature, deliberately preserving ctx->rawMsg. Once a context has been used with OSSL_SIGNATURE_PARAM_MESSAGE_ENCODING=0, every subsequent sign_init/verify_init/digest_*_init stays in FIPS 205 internal-interface mode unless the caller explicitly sets it back to 1.

In that mode wp_slhdsa_sign/wp_slhdsa_verify call wc_SlhDsaKey_SignMsg*/VerifyMsg, which skip M' construction entirely — the context string is silently dropped and there is no domain separation.

Persisting the context string, deterministic flag and test entropy is defensible; persisting the algorithm variant is a different class of stickiness. The sibling disagrees outright: wp_mldsa_sig.c:332-334 explicitly resets its analogous external-mu flag on every init, commented "Match OpenSSL: re-init clears external-mu but persists the context string, deterministic flag and test-entropy." Both files carry a "Match OpenSSL" comment and cannot both be right.

The persistence is asserted by test_slhdsa_reinit_null_key (test/test_slhdsa.c:1119-1191), so it is intentional — but should be confirmed against OpenSSL's slh_dsa_sig.c before shipping.

Suggestion:

Recommendation: Reset ctx->rawMsg = 0 mirroring wp_mldsa_sig.c's ctx->mu = 0, and update the test. If OpenSSL genuinely persists msg_encode, cite that so the divergence is documented.

🟡 [Medium] PQC gating produces degenerate artifacts: the example runs keygen in verify-only builds, can compile to an empty body that exits 0, and the interop arrays can degenerate to empty initialisers
💡 SUGGEST bug

Sites: examples/pqc_openssl_example.c:151-152,262; test/standalone/tests/pqc_interop/test_pqc_interop.c:658,1016; examples/pqc_openssl_example.c:242-280
The SLH-DSA section is gated only on WOLFPROV_HAVE_SLHDSA (the configure -D flag) plus EXAMPLE_SLH_DSA_NAME, not on WP_HAVE_SLHDSA_PRIVATE. include/wolfprovider/settings.h:256-260 defines WP_HAVE_SLHDSA_PRIVATE only when WOLFSSL_SLHDSA_VERIFY_ONLY is absent; the provider offers verify only otherwise.

run_signature() unconditionally calls generate_key(), so on a verify-only wolfSSL the example builds (BUILD_PQC_EXAMPLE is true whenever --enable-slhdsa is passed) but can never succeed. This PR adds exactly that config: .github/workflows/wolfssl-versions-pqc.yml:93-95 has the master (SLH-DSA verify-only) row with slh-config: yes,sha2,verify-only. It only avoids failing because line 145 excludes expect == 'slhdsa-verify' from the run step — the binary is knowingly built in a configuration where it cannot pass.

Also, at test/standalone/tests/pqc_interop/test_pqc_interop.c:658,1016 (Low, review scan) — pqc_interop SLH-DSA arrays can degenerate to empty initialisers with no guard:

Both the slhdsa[] name array (1016) and the map[] table in slhdsa_name_to_param (658) list only SHA2-128f, SHA2-192f and SHAKE-128f, each behind its own #ifdef, inside an outer #if defined(WP_HAVE_SLHDSA) && defined(WP_HAVE_SLHDSA_PRIVATE).

WP_HAVE_SLHDSA can be defined while none of those three sets is — settings.h:261-298 enables each set independently from wolfSSL's WOLFSSL_SLHDSA_PARAM_* defines — yielding const char* slhdsa[] = { };. An empty initialiser list is a GNU extension and a constraint violation in strict C, erroring under -Wpedantic.

test_slhdsa.c avoids exactly this by gating slhdsa_sets[] on WP_SLHDSA_TEST_SETS (test/unit.h:60-71); this file has no equivalent gate.

Also, at examples/pqc_openssl_example.c:242-280 (Low, review scan) — PQC example can compile to an empty body and exit 0, giving false CI confidence:

configure.ac:207-210 makes BUILD_PQC_EXAMPLE true when any of ML-KEM / ML-DSA / SLH-DSA is enabled, but main() only executes a body under WOLFPROV_HAVE_MLKEM, WOLFPROV_HAVE_MLDSA, or (WOLFPROV_HAVE_SLHDSA && EXAMPLE_SLH_DSA_NAME).

EXAMPLE_SLH_DSA_NAME comes from the WP_HAVE_SLH_DSA_* macros that settings.h:261-298 derives from wolfSSL's parameter-set defines. An SLH-DSA-only build whose wolfSSL enables no set from that list still satisfies settings.h's #error check (WP_HAVE_SLHDSA is defined) but leaves EXAMPLE_SLH_DSA_NAME undefined — so the example loads the provider, exercises nothing, and returns 0. CI reads that as a pass.

Suggestion:

Recommendation: Add WP_HAVE_SLHDSA_PRIVATE to the guard (and to the run_signature #if at 151-152), or have the example import a fixed public key and verify only when private-key support is absent.

[test/standalone/tests/pqc_interop/test_pqc_interop.c:658,1016] Add an outer guard mirroring WP_SLHDSA_TEST_SETS, or a trailing NULL sentinel so the array is never empty.

[examples/pqc_openssl_example.c:242-280] Track whether any algorithm ran and return non-zero with a diagnostic when nothing executed.

🟡 [Medium] DH key data is guarded by two different mutexes: wp_dh_encode takes the new keyMutex while wp_dh_sync_priv_to_key still uses the reference-count mutex
💡 SUGGEST bug

The PR adds wp_Dh::keyMutex specifically to guard key data ("Mutex for key data access and synchronization", line 72), and wp_dh_encode locks it. But the new wp_dh_sync_priv_to_key — the code that actually mutates key data via wc_DhImportKeyPair — locks wp_dh_get_mutex(), which returns &dh->mutex, the reference-count mutex (2650-2652). Two different mutexes guard the same data.

Consequences: (a) the helper's lock provides no exclusion against anything holding keyMutex, so it is decorative for any caller other than wp_dh_encode, which already holds it; (b) it needlessly serializes encoding against wp_dh_up_ref/wp_dh_free refcount updates.

No deadlock exists today — the only nesting is keyMutex→mutex, and wp_dh_free/wp_dh_up_ref take only mutex, so there is no inversion. But it is a latent lock-ordering hazard and implies the helper is self-synchronizing when it is not.

Also, at src/wp_dh_kmgmt.c:2648-2696 (Medium, concurrency scan) — wp_dh_sync_priv_to_key guards dh->key with the reference-count mutex, not the new keyMutex:

This PR adds a second mutex, keyMutex, documented as the "Mutex for key data access and synchronization" (struct wp_Dh, lines 69-74), and wp_dh_encode() takes key->keyMutex around the whole encode. But the new helper that actually MUTATES the key material — wp_dh_sync_priv_to_key(), which calls wc_DhImportKeyPair() on dh->key — locks wp_dh_get_mutex(dh), and wp_dh_get_mutex() returns &dh->mutex, the REFERENCE-COUNT mutex used by wp_dh_up_ref() (line 457) and wp_dh_free(). Two different mutexes are therefore associated with the same data, which provides no mutual exclusion. Today this does not produce an observable race only by accident: every caller of wp_dh_sync_priv_to_key() (wp_dh_encode_pki_size:2714, wp_dh_encode_epki_size:2780, wp_dh_encode_epki:2822) is reached exclusively from wp_dh_encode(), which already holds keyMutex, so the outer keyMutex is what actually provides exclusion and the dh->mutex acquisition is redundant. Two consequences remain: (1) the invariant "keyMutex protects dh->key" is silently violated, so the first caller added that reaches the helper without keyMutex gets zero mutual exclusion against a concurrent encode; (2) it establishes a nested lock order keyMutex -> mutex, so key-material mutation now runs with the refcount mutex held. No current path takes mutex -> keyMutex, so there is no deadlock today, but any future code that locks both in the natural order (refcount first, then key) deadlocks against an in-flight encode. Note that wp_dh_encode_spki_size() performs the equivalent mutation of dh->key with NO mutex of its own, relying purely on the caller's keyMutex — an inconsistency reinforcing that keyMutex, not dh->mutex, is the intended lock.

Suggestion:

Recommendation: Either make wp_dh_get_mutex() return &dh->keyMutex and drop the redundant nested lock, or remove the lock and document that the caller must hold keyMutex.

[src/wp_dh_kmgmt.c:2648-2696] Make wp_dh_get_mutex() (or a new wp_dh_get_key_mutex()) return &dh->keyMutex so the same mutex wp_dh_encode() takes is the one guarding dh->key. Because wp_dh_encode() already holds keyMutex when the helper runs and wolfSSL mutexes are not recursive, the helper should instead take no lock and document "caller must hold keyMutex", or the lock should be pushed down into the helper and removed from wp_dh_encode(). Keep dh->mutex strictly for refCnt, and give wp_dh_encode_spki_size() the same documented contract.

🔵 [Low] New tests assert the wrong thing: three robustness defects in the test_slhdsa.c helpers, and the cipher-rejection tests report the wrong cause while leaving the error queue dirty
💡 SUGGEST test

Sites: test/test_slhdsa.c:123,239,950; test/test_ecc.c:960-972, test/test_rsa.c:2396-2408

1. slhdsa_get_raw reads *out without ever writing it on the sizing-failure path (line 123). If the sizing call at 114 fails, err becomes 1, the malloc at 116 is skipped, and *out is never assigned — yet 123 dereferences it. The [out] contract entitles a caller to pass an uninitialised pointer. All current call sites pre-initialise to NULL so it does not fire today, but the sibling slhdsa_sign_with (1299) sets *sig = NULL up front, so the file is internally inconsistent.

2. test_slhdsa_keygen compares with the wrong length (line 239). p2 is allocated from its own independent sizing query, and p2Len (229) is never validated against p1Len or set->pubKeySize. memcmp then reads p1Len bytes from p2. If the provider ever returned a short public key for the second key — the regression class this suite exists to catch — this overreads p2's allocation. It is also the only comparison in the file with no PRINT_ERR_MSG.

3. Allocation failure turns the public-only-sign negative test into a silent pass (line 950). The && chain sets err only when all three sub-expressions succeed. If OPENSSL_malloc(sigLen) returns NULL the chain is false and err stays 0, so the test reports PASS even though a public-only key just initialised a signing operation and produced a signature length. An OOM must not be indistinguishable from "the provider correctly refused".

Also, at test/test_ecc.c:960-972, test/test_rsa.c:2396-2408 (Low, review scan) — Cipher-rejection tests report the wrong cause and leave the error queue dirty:

Two issues in the same new blocks.

Misattributed failure. The tests fold three distinct outcomes into one boolean and print a single message describing only one of them. If the encoder context cannot be built, or OSSL_ENCODER_CTX_set_cipher fails, the test reports "encoder accepted a cipher" when it never got as far as offering one.

Dirty error queue. Both blocks deliberately provoke a failure (the expected outcome is that OSSL_ENCODER_to_data fails and pushes errors) but neither calls ERR_clear_error() afterwards. The companion helper added in the same PR, test_pki_cipher_encrypts (test/test_pkey.c:367, 442), does clear after each intentional failure. Leftover entries get attributed to whatever test prints errors next.

Suggestion:

Recommendation: Add *out = NULL; as the first statement of slhdsa_get_raw; compare p1Len != p2Len before memcmp with a PRINT_ERR_MSG; and unroll the public-only chain into the file's usual if (err == 0) cascade so an OOM is distinguishable from a correct refusal.

[test/test_ecc.c:960-972, test/test_rsa.c:2396-2408] Split setup failures from the assertion, assert only on OSSL_ENCODER_to_data() returning 1, and add ERR_clear_error() in both files.

🔵 [Low] wp_internal.c PKCS#8 helper hygiene: wp_encrypt_key_pkcs8_size missed the NULL-argument hardening, the RNG unlock is indented as if outside its enclosing block, and wp_cipher_from_params' doc comment omits the new clear-on-failure semantics
💡 SUGGEST convention

Sites: src/wp_internal.c:997-1053; src/wp_internal.c:1143-1175; src/wp_internal.c:906-916
The PR adds (outLen == NULL) || (!WP_FITS_WORD32(*outLen)) || (pwCb == NULL) validation to wp_encrypt_key_pkcs8 (1119-1122) but leaves the paired size function with only the pre-existing if (cipher == 0) check. wp_encrypt_key_pkcs8_size dereferences provCtx unconditionally at 1029/1036 via wp_provctx_get_rng() and writes *outLen at 1046 with no NULL test.

All current callers pass valid pointers, so this is a consistency gap rather than a live crash — but the two are a matched pair declared together in internal.h.

Also, at src/wp_internal.c:1143-1175 (Low, review scan) — RNG unlock in wp_encrypt_key_pkcs8 is indented as if outside its enclosing if (ok) block:

The lock was hoisted out of the if (ok) { block (1135-1142) but the matching unlock at 1161-1166 is still inside it, at 4-space indentation while surrounding statements are at 8. Reading the file it looks like the unlock sits at function scope next to the lock; it does not.

Functionally correct — the lock is only taken when ok == 1 and success preserves ok, so the following if (ok) is always entered. But the layout invites a future edit to silently leak the mutex, and it is inconsistent with wp_encrypt_key_pkcs8_size, where the PR correctly hoisted the unlock out (1041-1046).

Also, at src/wp_internal.c:906-916 (Low, review scan) — wp_cipher_from_params doc comment does not describe the new clear-on-failure semantics:

The rewrite introduces three behaviors the unchanged Doxygen block does not mention: (1) when the cipher param is absent, both outputs are left untouched so a previously configured cipher persists; (2) p->data == NULL now clears *cipher and returns success instead of failing; (3) every failure path clears both outputs. The comment still says only "A parameter with the name of the cipher may not be in the array."

Separately, the (cipherName != NULL) guards on all three paths are dead code — all six call sites pass &ctx->cipherName.

Suggestion:

Recommendation: Extend the guard so the size query and the encrypt call reject the same bad inputs.

[src/wp_internal.c:1143-1175] Move the unlock out of the if (ok) block to mirror wp_encrypt_key_pkcs8_size, or at minimum re-indent.

[src/wp_internal.c:906-916] Document the absent / NULL-value / failure semantics in the header comment.

🔵 [Low] Message-API entry points omit WOLFPROV_ENTER/WOLFPROV_LEAVE
🔧 NIT convention

Every other dispatched entry point in this file wraps its body in WOLFPROV_ENTER/WOLFPROV_LEAVE, and wp_mldsa_sig.c's wp_mldsa_verify_message_final (1112-1130) does too. These three are bare forwarders with no tracing, so a failure inside the OpenSSL 3.6 message API — in particular the "no staged signature" rejection at 638 — is invisible in a WOLFPROV_DEBUG log.

Suggestion:

Recommendation: Add WOLFPROV_ENTER/WOLFPROV_LEAVE to the three functions.

🔵 [Low] wp_slhdsa_free still permanently leaks the key object, its private key material and both mutexes when wc_LockMutex(&refMutex) fails
🔧 NIT

When the reference-count mutex cannot be acquired, wp_slhdsa_free() hardcodes cnt = 1 rather than decrementing, so the reference is never released and the object is never reclaimed. Because the function returns void there is no way to signal this to the caller (OSSL_FUNC_KEYMGMT_FREE and OSSL_FUNC_ENCODER_FREE_OBJECT both dispatch straight to it), and the object is unreachable afterwards. The leaked block contains the raw SLH-DSA private key (SlhDsaKey.sk, up to 4n bytes) which is never zeroized, plus keyMutex and refMutex which are never wc_FreeMutex()'d. Every subsequent free attempt on any surviving reference hits the same failure, so the leak is permanent and repeatable. Note the sibling wp_dh_free() uses cnt = dh->refCnt in the same situation; the SLH-DSA constant 1 also loses the diagnostic value of the real count. Trigger requires wc_LockMutex to fail, which is why this is Low.

Recommendation: Keep the conservative no-free behaviour, but make it observable: log at error level (not debug) so the leak is diagnosable, and consider replacing the hand-rolled refCnt/refMutex pair with an atomic counter (C11 atomic_fetch_sub or OpenSSL's CRYPTO_REF_COUNT) so reference counting cannot fail at all and refMutex can be dropped entirely.

🔵 [Low] wp_slhdsa_export_object locks a borrowed key pointer from the decoder reference without holding a reference on it
🔧 NIT

The decoder EXPORT_OBJECT entry dereferences the OSSL_OBJECT_PARAM_REFERENCE blob into a raw wp_SlhDsa* and hands it straight to wp_slhdsa_export(), which immediately calls wp_lock(wp_slhdsa_get_mutex(slhdsa)) at line 949. No reference is taken (no wp_slhdsa_up_ref) for the duration, so if the owning thread drops the last reference concurrently, wp_slhdsa_free() runs wc_FreeMutex(&slhdsa->keyMutex) and OPENSSL_free(slhdsa) while this thread is about to wc_LockMutex the same mutex — a use-after-free on a freed pthread mutex, which on glibc typically hangs or aborts rather than failing cleanly. The same borrowed-pointer shape exists in wp_slhdsa_load() (lines 520-526). Both match the existing ML-DSA pattern and the OpenSSL decoder contract makes the caller responsible for keeping the reference alive, which is why this is Low — but the SLH-DSA object is the first of the two whose mutex gets destroyed on free, so the failure mode is now a deadlock/abort rather than a benign read of freed memory.

Recommendation: Take a reference for the duration of the export: wp_slhdsa_up_ref(slhdsa) before wp_slhdsa_export() and wp_slhdsa_free(slhdsa) after, so the object (and therefore keyMutex) cannot be destroyed underneath the lock acquisition.